Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-remove dependency on Guava #88

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

john-h-kastner-aws
Copy link
Contributor

Requires the following changes:

  • Use copyOf and unmodifiableSet functions to replace ImmutableSet. Prefer using copyOf because it creates an unmodifiable copy of the set or list, so mutation to the input does not reflect in the new object. unmodifiableSet can still be used to return an immutable view of a set without causing any copying.

  • Roll our own memoizing supplier. This change should be reviewed by someone with more Java knowledge, but, AFAIK, the behavior should be the same.

Issue #, if available:

#86

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* Use `copyOf` and `unmoidifiableSet` functions to replace
  `ImmutableSet`
* Roll our own memoizing supplier
@john-h-kastner-aws john-h-kastner-aws linked an issue Feb 19, 2024 that may be closed by this pull request
2 tasks
@andrewmwells-amazon
Copy link
Contributor

Maybe ask Lucas to review?

@@ -34,7 +32,15 @@ public final class EntityTypeName {
protected EntityTypeName(List<String> namespace, String basename) {
this.namespace = namespace;
this.basename= basename;
this.entityTypeNameRepr = Suppliers.memoize(() -> getEntityTypeNameRepr(this));
this.entityTypeNameRepr = new Supplier<String>() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anyone is wondering why this is stalled: The Guava memoizing supplier is thread safe, but this implementation would not be. We don't make an explicit guarantees about thread safety of the Java bindings, but out Rust implementation is thread safe, so I don't want to break that here, and I haven't had the time to do this properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noting the current status @john-h-kastner-aws.

Reviewing the content of this class, is the purpose of using a Supplier to defer to the native JNI call until it is actually needed? Since instances of this class are effectively immutable, it seems like calling getEntityTypeNameRepr() multiple times is not necessarily a problem for the Java toString() representation, unless there is a state concern on the Rust side.

@john-h-kastner-aws john-h-kastner-aws marked this pull request as draft March 28, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing Guava dependecy
3 participants